-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add login page (Backend) #16
base: main
Are you sure you want to change the base?
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
4c06da4
to
e5801a4
Compare
27e4ea7
to
50b9e0b
Compare
639e9a0
to
45c472b
Compare
8b1ce1e
to
1ec7937
Compare
Need some time to tidy up everything, but generally everything should be done for this PR |
66c6be0
to
1ec6c17
Compare
22b2cbb
to
3c6aff8
Compare
@@ -0,0 +1,349 @@ | |||
use super::prelude::*; | |||
|
|||
pub type HttpClient = ContextWrapper< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain what's going on with ClientContext
here? Why do you declare the Client
without context (DropContextService
) and then wrap it with context? What is the reason that Client
was not declared directly with the context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory, we can wrap any context with any context provider (for example, the one that reads\writes context from\to file, and not that just wraps the context and keeps it in-memory). Practically, It could be solved with generic types and some methods, not the whole wrapper, but since that was the part of auto-generated code, I decided to keep it this way
Also we can replace the context on specific calls, not changing the wrapped one, but I fail to see the situation where that's actually useful (and the wrapper part too)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that generics are used for better flexibility. But here a specific type with all substitutions is declared. Why are additional layers needed here? Why can't it be written like this:
pub type HttpClient = ContextWrapper<
Client<
hyper::Client<HttpConnector, Body>,
ClientContext,
Basic,
>,
ClientContext,
>;
ae6e12d
to
3c95e55
Compare
278f19f
to
35d21ae
Compare
@@ -5,8 +5,10 @@ Bob Management GUI changelog | |||
## [Unreleased] | |||
|
|||
#### Added | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary empty line
} | ||
|
||
#[derive(Clone)] | ||
pub struct BobClient<Client: ApiNoContext<ClientContext> + Send + Sync> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps, it is better to call it BobClusterClient
, because inside it contains clients for all nodes
@@ -136,22 +181,38 @@ impl<V, D, S, B> Deref for ContextRouter<V, D, S, B> { | |||
} | |||
} | |||
|
|||
#[cfg(all(feature = "swagger", debug_assertions))] | |||
pub trait RouterApiExt<S = (), B = Body, E = Infallible> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see no difference between this trait and the one below. Looks like the separation based on cfg
is not needed on trait declaration level
} | ||
} | ||
|
||
#[cfg(all(feature = "swagger", debug_assertions))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The separation of implementations occurs at different levels. Here the struct
level is used, and above - the function level. It is better to unify places, where cfg
is applied. It seems that it is better to make a separation at the function level here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only difference seems to be the call to check_api
, I suggest to cfg
only the call and the function itself
} | ||
|
||
#[cfg(not(all(feature = "swagger", debug_assertions)))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementations are also look identical
Ok(res) | ||
} | ||
|
||
#[async_trait] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like async_trait
can be removed here
} | ||
} | ||
|
||
#[async_trait] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like async_trait
is not needed here
let connector = Connector::builder(); | ||
|
||
let client_service = match scheme.as_str() { | ||
"http" => HyperClient::Http(hyper::client::Client::builder().build(connector.build())), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about https, can it be added? bob
technically supports it
|
||
let cluster: HashMap<NodeName, Arc<_>> = nodes | ||
.iter() | ||
.filter_map(|node| HttpClient::from_node(node, &bob_data.hostname, context.clone())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API ports can be different on each node, and they are not known to other nodes. There should be a way to override default ports. In bob-tools
it's done with argument api-port
, Override default api port for the node. E.g. node1:80,node2:8000. Wildcard char (*) can be used to set port for all nodes.
} | ||
} | ||
|
||
/// Probes connection to the Bob's main connected node |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment out of sync?
|
||
// NOTE: Can (and should) the API interface mutate?.. | ||
/// Connection, | ||
main: Arc<Client>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason to have single main node? Why not any other node in cluster?
partly resolves the #2 issue